feat(instruction-placement): add detect.sh identity subcommand - #5486

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

Claude finished @kyle-sexton's task in 2m 56s —— View job

Code review: detect.sh identity subcommand

• Gather context (PR body, diff, prior Codex review comments)
• Review detect.sh identity subcommand changes
• Review detect.test.sh new coverage
• Review doc/SKILL.md updates for consistency
• Verify whether the two prior Codex P2 findings (sha256 fallback, path canonicalization) are already addressed by the latest commit
• Post findings

No blocking findings. I reviewed the full diff (git diff origin/main...HEAD) across all 8 changed files, with focus on plugins/instruction-placement/scripts/detect.sh.

Both P2 issues Codex raised against bc14f19 are resolved by the latest commit, 9d0f062:

• Missing-sha256-utility fallthrough — detect.sh now checks for sha256sum/shasum up front and exits 2 via id_die if neither exists (detect.sh:141-143), instead of the old silent-fallthrough. Covered by the PATH=/nonexistent test case.
• Noncanonical --file spellings (./x, ././x, docs/../x, docs//x, backslash segments) — the old single ${ID_FILE#./} strip is replaced with an explicit reject on */./* | */../* | *//* | *\\* (detect.sh:139-141), with a corresponding test loop over all five bad spellings.

I traced the fix by hand against both cases (e.g. docs/../CLAUDE.md → wrapped /docs/../CLAUDE.md/ contains the literal /../ substring → rejected; a legitimate hidden-dir path like .claude/rules/foo.md does not match */./* since no two-slash-with-single-dot substring is present, so no new false positive). The heading-path walk in emit_identity (nearest preceding SECTION record at a strictly lower level, walked up to level 1) also matches the SECTION record field order emitted by emit_file_facts (path start end level heading), so the tab-field parsing (read -r kind _ start _ level head) lines up correctly.

Docs (findings-artifact.md, consumer-config.md, both SKILL.md files) consistently redirect the hand-derivation instructions to detect.sh identity, and the CHANGELOG/version bump match the described change.

Not independently verified: the sandbox's auto-permission classifier declined my attempts to execute bash (including detect.test.sh), so the PR's claimed test results (55 cases / 0 failures) and the check-changelog-parity.sh / validate-plugins.sh runs are author-claimed, not something I ran myself. My analysis above is static (diff reading + manual trace of the shell logic), not an execution-verified pass.

Claude finished @kyle-sexton's task in 2m 12s —— View job

Security review — /review:security-review

• Gather context (diff, changed files, existing Codex review comments)
• Run /review:security-review for PR feat(instruction-placement): add detect.sh identity subcommand #5486 at HEAD 9d0f062
• Post findings

Scope reviewed: origin/main...HEAD (8 files, +256/-10), focused on
plugins/instruction-placement/scripts/detect.sh
since it's the only file with security-relevant surface (path handling, hashing, new subcommand argument parsing). The doc/skill changes only tighten wording to route derivation through the new subcommand — no instruction deletions or weakenings.

Result: no new security findings.

Codex's two prior P2 findings on this PR (noncanonical --file paths defeating deterministic hashing, and silent success when no SHA-256 utility is present) are both already fixed by the latest commit, 9d0f062 ("identity rejects noncanonical paths and missing sha256 tools"):

• detect.sh#L139-141 rejects --file values containing /./, /../, //, or a backslash segment (checked as "/$ID_FILE/" against those glob patterns), which covers ./CLAUDE.md, ././CLAUDE.md, docs/../CLAUDE.md, docs//x.md, and root-escaping ../... spellings — all now a usage error (exit 2) instead of hashing to a different finding_id or escaping --root.
• detect.sh#L142-143 now checks for sha256sum/shasum up front and exits 2 if neither is present, instead of previously falling through to shasum unconditionally and emitting a malformed IDENTITY record with an empty anchor/id on hosts with neither tool.
• detect.test.sh adds direct regression coverage for both: the noncanonical-path loop (./CLAUDE.md ././CLAUDE.md docs/../CLAUDE.md docs//x.md 'docs\x.md') and a PATH=/nonexistent run asserting exit 2 "not a bogus id".

One residual, non-exploitable edge case I considered and am not raising as a finding: a Git-Bash-style drive path like C:/repo/CLAUDE.md doesn't match the /./, /../, //, \ patterns, so it isn't rejected by the canonical-path check. However it still resolves as a plain relative path under --root after the script's cd "$ROOT" (detect.sh#L179-180), and Windows/Linux filesystems disallow : in a real path component, so it can never resolve to an existing file — there's no actual root-escape or collision, just an inert string that fails the later -f check. Not a concrete exploit path, so it doesn't clear this lane's high-signal bar.

I ran through the rest of the diff (plugin.json version bump, CHANGELOG.md, and the three doc/SKILL.md files) — none introduce authorization, injection, or secret-handling concerns; they only redirect prose to call the new subcommand instead of hand-deriving values.

Branch ·

Claude finished @kyle-sexton's task in 40s —— View job

Security review — /review:security-review

• Gather context (PR body, diff, prior Codex/Claude review comments)
• Invoke /review:security-review for PR feat(instruction-placement): add detect.sh identity subcommand #5486 at HEAD 11834f7
• Post findings

Result: no security findings.

This head (11834f7) differs from the previously fully-reviewed head (9d0f062, reviewed here) by exactly one line in detect.sh:

   case "/$ID_FILE/" in
   */./* | */../* | *//* | *\\*) id_die "--file must be a canonical relative path (no ., .., empty or backslash segments): $ID_FILE" ;;
+  *) ;;
   esac

This is a no-op default arm added to satisfy shellcheck SC2249 (require a default case). It changes no behavior — the canonical-path rejection (/./, /../, //, backslash segments) and the sha256sum/shasum availability check on the surrounding lines are unchanged from the previously reviewed commit, where both were already confirmed to close out Codex's two P2 findings (noncanonical path aliasing, silent success with no SHA-256 utility).

No new security-relevant surface is introduced by this diff. The rest of the PR (doc/SKILL.md redirects, CHANGELOG, plugin version bump) is unchanged since the last review and carries no security surface.
· Branch

Claude finished @kyle-sexton's task in 2m 51s —— View job

Code review (/review:code-review)

• Gather context (PR body, diff, prior Codex/Claude review comments)
• Review detect.sh identity subcommand (argument parsing, path canonicalization, sha256_hex, emit_identity heading-path walk)
• Review detect.test.sh new coverage (55 cases including the two golden vectors and skipped-level walk)
• Review the four doc updates (findings-artifact.md, consumer-config.md, both SKILL.md files) for consistency with the implemented flags/formula
• Post findings

No new high-signal findings.

This lane's scope is correctness/maintainability (security is a separate lane, and one already ran on this PR). I independently traced the full identity path in detect.sh rather than relying on the prior comments' description of it:

• The canonicalization check (case "/$ID_FILE/" in */./* | */../* | *//* | *\\*), L139-142) correctly rejects ./x, ././x, docs/../x, docs//x, and backslash segments, while not falsely rejecting legitimate paths like .claude/rules/foo.md or a filename containing .. outside a path-segment position (e.g. a..b.md) — confirmed by hand-checking the wrapped-string substring logic, not just reading the diff.
• The upfront sha256sum/shasum check (L143-144) closes the silent-fallthrough gap Codex flagged against bc14f19.
• emit_identity's heading-path walk (L488-495) — nearest preceding SECTION at a strictly lower level — correctly handles a skipped level (H3 directly under H1) and correctly skips a same-level sibling rather than treating it as an ancestor; both are exercised by detect.test.sh's skipped.md cases.
• The finding_id join order (check␟claim␟surface␟anchor, L511) matches findings-artifact.md's documented [check, claim, surface, anchor] formula, and surface there is ID_FILE, consistent with the doc.
• The four doc updates (findings-artifact.md, consumer-config.md, skills/audit/SKILL.md, skills/delta/SKILL.md) all point at the same --file/--start/--lane/--destination flag set the script actually implements — no drift between prose and code.

The one commit added since the prior full review (11834f7) is a one-line no-op *) ;; case arm for shellcheck SC2249 — mechanically inert, confirmed by reading its diff directly.

Not independently verified: the sandbox's auto-permission classifier declined my bash execution attempts (both git fetch to rebuild a full merge-base diff against a shallow clone, and running detect.test.sh directly), so the PR's claimed 55/0 test result, check-changelog-parity.sh, and validate-plugins.sh runs remain author-claimed. My review is a static read of the current file content plus manual trace of the shell logic, corroborating (not just restating) the prior Codex/Claude passes.

Branch